refactor: better configuration - #2854
Conversation
- Introduced `guard-no-legacy-config.mjs` to prevent direct usage of `process.env` in core packages, ensuring all environment variables are declared in `app-config`. - Added a new script `sync-config-docs.ts` to automatically generate and update the declared configuration documentation from `appConfigSchema`. - Updated `run-guards.ts` to include the new guards for configuration management. - Enhanced the `agent-friction-report.mjs` with a new pattern to track environment variable sprawl.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🟡
Review Details
Code Review Summary
PR #2854 centralizes environment-backed configuration in app-config, adds generated configuration documentation, and tightens guard coverage across core consumers. The overall direction is sound: a declared schema, explicit precedence layers, shared resolvers, and executable guards reduce duplicated parsing and make configuration discoverable. The new tests around layer merging and origin resolution are helpful, and the legacy-env guard passes in this checkout.
Key Findings
🟡 MEDIUM
- The committed generated configuration table is stale, so the new documentation guard fails on a clean checkout.
- The legacy-env guard has broad directory and namespace exemptions that allow undeclared configuration reads to bypass the intended default-deny policy.
- The generated worker shell rebuilds origin configuration from
process.env, bypassing higher-precedencedefineAppConfig()values. - Webhook self-dispatch now uses the generic resolver and can ignore the inbound request host when an ambient configured URL exists.
The PR is standard risk: it changes shared core configuration and deployment behavior, but not authentication or payment logic.
🧪 Browser testing: Will run after this review (PR touches UI code)
| "server/credential-provider.ts", | ||
| "server/request-context.ts", | ||
| // Build/deploy tooling composes env for a child, rather than reading config. | ||
| "deploy/", |
There was a problem hiding this comment.
🟡 Limit legacy-config guard directory exemptions
Exempting every file under deploy/, vite/, cli/, and scripts/ lets future runtime configuration consumers in those directories read process.env without declaring an app-config field. Limit the exemption to the specific child-environment producer modules or relevant lines so CLI and script consumers remain covered by the default-deny guard.
Additional Info
Found by 1 of 4 review agents; overlaps the guard's documented resolver-only policy.
| "LAMBDA_TASK_ROOT", | ||
| ]); | ||
|
|
||
| const PLATFORM_PREFIXES = ["AWS_", "npm_", "GITHUB_", "VITEST", "NETLIFY_"]; |
There was a problem hiding this comment.
🟡 Restrict platform namespace exemptions
Exempting every AWS_*, GITHUB_*, NETLIFY_*, npm_*, and VITEST* variable allows undeclared application settings—and credentials such as GITHUB_TOKEN—to bypass the guard. Use an explicit allowlist of genuinely host-owned keys rather than whole namespaces.
Additional Info
Found by 1 of 4 review agents; confirmed from the guard implementation.
| // app.url / workspace.* in app-config (worker bundles a string copy; it | ||
| // can't import them). Impersonal values only — this ships into the | ||
| // CDN-cached shell. | ||
| const env = globalThis.process?.env || {}; |
There was a problem hiding this comment.
🟡 Honor explicit app configuration in worker shell output
The generated worker origin projection reconstructs values directly from globalThis.process.env, while the normal SSR projection uses getAppConfig() and honors higher-precedence defineAppConfig() values. An app that sets app.url or workspace.* in code while environment aliases differ can therefore send conflicting server and browser origins on worker deployments; pass the resolved app-config values into this generated path or otherwise preserve the same precedence ladder.
Additional Info
Found by 2 of 4 review agents; confirmed by the differing resolution paths.
| `http://localhost:${process.env.PORT || 3000}`, | ||
| ); | ||
| } | ||
| return resolveSelfDispatchBaseUrl(event); |
There was a problem hiding this comment.
🟡 Preserve request-aware webhook self-dispatch fallback
Delegating resolveBaseUrl(event) to resolveSelfDispatchBaseUrl(event) changes webhook behavior when an ambient configured URL exists: the shared resolver can prefer that URL before considering the inbound request host. The targeted webhook test fails by dispatching to the configured local origin instead of the fixture host; preserve the webhook-specific request-aware fallback or derive and pass the request base URL explicitly.
Additional Info
Found by 1 of 4 review agents; targeted webhook test failure was reported and the resolver behavior is confirmed in surrounding code.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The config-docs generator emitted unpadded markdown table rows while oxfmt reformats them padded, so guard:config-docs and oxfmt --check could never both pass. Emit the padded form the formatter produces. bookings-preview.spec.ts asserted on a hardcoded Monday without pinning the clock, so it began failing once that date arrived and slot generation dropped every past slot.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Builder reviewed your changes — no new findings
Review Details
Incremental Code Review Summary
The latest PR commits update the configuration documentation generator to emit formatter-stable Markdown and add a calendar booking-preview regression test with deterministic time handling. The previously reported generated-docs issue is fixed and its review thread was resolved. The remaining earlier configuration guard, worker-shell precedence, and webhook self-dispatch comments are unchanged and were intentionally not reposted.
Four parallel incremental reviews found no new confirmed bugs. Validation reported by the reviewers includes passing guard:config-docs, guard:no-legacy-config, app-config and origin tests, the calendar booking-preview test, and formatting checks. This incremental delta contains only documentation, guard/generator, and test changes, with no new user-facing browser behavior.
Risk remains standard because the overall PR changes shared core configuration.
🧪 Browser testing: Skipped — incremental delta is non-frontend (documentation, guard/generator, and test-only changes).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 participants
guard-no-legacy-config.mjsto prevent direct usage ofprocess.envin core packages, ensuring all environment variables are declared inapp-config.sync-config-docs.tsto automatically generate and update the declared configuration documentation fromappConfigSchema.run-guards.tsto include the new guards for configuration management.agent-friction-report.mjswith a new pattern to track environment variable sprawl.